Skip to content

ffi: wrap BN_hex2bn in C to avoid BIGNUM** ctypes binding#2

Open
georgeee wants to merge 1 commit intoo1labsfrom
georgeee/fix-hex2bn-bindings
Open

ffi: wrap BN_hex2bn in C to avoid BIGNUM** ctypes binding#2
georgeee wants to merge 1 commit intoo1labsfrom
georgeee/fix-hex2bn-bindings

Conversation

@georgeee
Copy link
Copy Markdown

@georgeee georgeee commented Mar 24, 2026

Summary

This patch replaces the direct ctypes binding of BN_hex2bn with a small C wrapper that returns a BIGNUM * instead of requiring a BIGNUM ** out-parameter.

This resolves a type incompatibility introduced by stricter compilers (e.g. GCC 15) and simplifies the OCaml call site, while preserving the exact runtime behavior.

The issue

The OpenSSL API for parsing hex into a bignum is:

int BN_hex2bn(BIGNUM **a, const char *str);

This function uses an out-parameter (BIGNUM **) with the following semantics:

  • *a == NULL → allocate a new BIGNUM

  • *a != NULL → reuse existing BIGNUM

  • return value:

    • 0 → failure
    • >0 → number of hex digits parsed

Original binding

Previously, this was bound in ctypes as:

let hex2bn = foreign "BN_hex2bn" Ctypes.(ptr t_opt @-> string @-> returning int)

and used as:

let p_ref = Ctypes.(allocate Bindings.Bignum.t_opt None) in
let _len = Bindings.Bignum.hex2bn p_ref hex in
match Ctypes.(!@) p_ref with
| Some p -> p
| None -> failwith ...

This corresponds to the C pattern:

BIGNUM *bn = NULL;
BN_hex2bn(&bn, hex);

which is the correct usage of the OpenSSL API.

Problem with newer compilers

Internally, ctypes generated a stub passing a void ** to BN_hex2bn, because the OCaml representation of the pointer was erased to void *.

This results in code like:

BN_hex2bn((void **), ...);

However, void ** is not compatible with BIGNUM ** in C. While historically tolerated, this is rejected by newer compilers (e.g. GCC 15):

error: passing argument 1 of ‘BN_hex2bn’ from incompatible pointer type
expected ‘BIGNUM **’ but argument is of type ‘void **’

This is a fundamental limitation of representing typed double pointers via ctypes.

Approach

Instead of binding BN_hex2bn directly, this patch introduces a small C wrapper:

BIGNUM *ocaml_BN_hex2bn(const char *str) {
  BIGNUM *bn = NULL;
  if (BN_hex2bn(&bn, str) == 0) {
    return NULL;
  }
  return bn;
}

This transforms the API from:

int BN_hex2bn(BIGNUM **a, const char *str);

into:

BIGNUM *ocaml_BN_hex2bn(const char *str);

The OCaml binding is then updated to:

let hex2bn = foreign "ocaml_BN_hex2bn" Ctypes.(string @-> returning t_opt)

and the call site simplified to:

match Bindings.Bignum.hex2bn hex with
| Some p -> p
| None -> failwith ...

Why this is correct and safe

1. Correct use of OpenSSL API

The wrapper uses the standard and documented allocation pattern:

BIGNUM *bn = NULL;
BN_hex2bn(&bn, str);

Passing a pointer initialized to NULL explicitly signals:

“allocate a new BIGNUM for me”

This is one of the two intended modes of BN_hex2bn.

2. Preserves behavior exactly

The old and new implementations are equivalent:

Aspect Old version New version
Input pointer *a == NULL via allocate None bn = NULL in wrapper
Allocation Performed by OpenSSL Performed by OpenSSL
Failure signal None after dereferencing pointer NULLNone
Success result Some p Some p

The only internal difference is that:

  • the old version ignored the return value
  • the new version checks == 0 and returns NULL

This does not change observable behavior.

3. Fixes the void ** vs BIGNUM ** issue

By removing BIGNUM ** from the FFI boundary entirely, the generated stubs now only deal with:

BIGNUM *

which is safely representable via ctypes as an opaque pointer.

This eliminates the incompatible pointer type error on newer compilers.

4. Keeps BIGNUM fully opaque

The OCaml side still treats BIGNUM * as an opaque handle:

  • no struct layout is assumed
  • no fields are accessed
  • memory is managed via existing OpenSSL functions (e.g. BN_free)

This matches OpenSSL’s design and avoids any reliance on internal representation.

5. Memory ownership is unchanged

In both versions:

  • BN_hex2bn allocates a new BIGNUM
  • ownership is transferred to the caller
  • the caller is responsible for eventually calling BN_free

The wrapper does not introduce any additional allocations or leaks.

6. Simpler and safer OCaml call site

The OCaml code no longer needs to:

  • allocate intermediate storage (ptr t_opt)
  • manually dereference out-parameters

This reduces surface area for mistakes and makes the intent clearer.

Conclusion

This patch:

  • resolves a real compiler incompatibility (void ** vs BIGNUM **)
  • preserves exact runtime semantics
  • uses the OpenSSL API in its intended way
  • simplifies the OCaml binding and call site
  • maintains correct ownership and opacity guarantees

It is a minimal and safe fix for the issue.

Replace direct ctypes binding of BN_hex2bn (BIGNUM**) with a small C
wrapper returning BIGNUM*. This avoids void**/BIGNUM** incompatibility
(GCC15+) and simplifies the OCaml call site.

No change in behavior: still returns None on failure and allocates a new
BIGNUM on success.
@georgeee
Copy link
Copy Markdown
Author

To validate the change compiles under GCC 15:

  1. Take Mina repository, checkout latest develop, run nix/pin.sh
  2. Checkout https://github.com/o1-labs/async_ssl
  3. Run nix develop mina
  4. From within that shell, run nix shell nixpkgs#gcc15 --command make

Before this patch, GCC 15 compilation was failing.

@glyh
Copy link
Copy Markdown
Member

glyh commented Mar 24, 2026

For different openSSL version, we need different version of this package. I wonder how is that reflected, or is it reflected at all?

I'm also concerned that we might need to maintain multiple version of this package if there's some future bugfixes we need to apply while supporting both.

@glyh
Copy link
Copy Markdown
Member

glyh commented Mar 24, 2026

@glyh
Copy link
Copy Markdown
Member

glyh commented Mar 24, 2026

I'd consider #3 instead if acceptable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants